build(deps): update Arrow/Parquet to `52.0`, object-store to `0.10` #10765

waynexia · 2024-06-02T08:34:27Z

Which issue does this PR close?

Closes #.

Rationale for this change

Previous one: Update Arrow/Parquet to 51.0.0, tonic to 0.11 #9613
Arrow release ticket: Release arrow-rs / parquet version 52.0.0 arrow-rs#5688

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

waynexia · 2024-06-05T09:30:04Z

Okay, the one last thing is to update Cargo.toml after arrow 52.0 is published. This PR itself is ready for review.

alamb · 2024-06-05T16:54:25Z

Epic!

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

andygrove · 2024-06-06T18:12:53Z

@waynexia It looks like you need to run cargo update in datafusion-cli to fix CI

datafusion/common/src/scalar/mod.rs

andygrove · 2024-06-06T18:30:01Z

datafusion/physical-plan/src/aggregates/mod.rs

-            common::collect(merged_aggregate.execute(0, task_ctx.clone())?).await?;
+        // enlarge memory limit in spill mode
+        let task_ctx = if spill {
+            new_spill_ctx(2, 2600)


Should this be configurable? Also could you add a comment explaining what is happening here?

@viirya could you also review?

This change has two stages to my understanding. The first partial aggr stage has a smaller limit, to meet the expectation that the partial plan has early emit.

datafusion/datafusion/physical-plan/src/aggregates/mod.rs

Lines 1504 to 1516 in 6846513

let expected = if spill {

vec![

"+---+---------------+-------------+",

"| a | AVG(b)[count] | AVG(b)[sum] |",

"+---+---------------+-------------+",

"| 2 | 1 | 1.0 |",

"| 2 | 1 | 1.0 |",

"| 3 | 1 | 2.0 |",

"| 3 | 2 | 5.0 |",

"| 4 | 3 | 11.0 |",

"+---+---------------+-------------+",

]

} else {

And the new lines enlarge the limit after that "early emit" check. Otherwise the merge aggr would fall because of insufficient memory. (at line 1554)

let result = common::collect(merged_aggregate.execute(0, task_ctx)?).await?;

The root cause of this change is somehow the memory requirement becomes larger. From my investigation, the biggest change compared to the previous is from the RawTable in GroupValuesPrimitive -- it needs about 1000 bytes more:

datafusion/datafusion/physical-plan/src/aggregates/group_values/primitive.rs

Lines 81 to 95 in 6846513

pub struct GroupValuesPrimitive<T: ArrowPrimitiveType> {

/// The data type of the output array

data_type: DataType,

/// Stores the group index based on the hash of its value

///

/// We don't store the hashes as hashing fixed width primitives

/// is fast enough for this not to benefit performance

map: RawTable<usize>,

/// The group index of the null value if any

null_group: Option<usize>,

/// The values for each group index

values: Vec<T::Native>,

/// The random state used to generate hashes

random_state: RandomState,

}

I hadn't realized that these literal values were just in tests ... thanks for the explanation

andygrove · 2024-06-06T18:30:23Z

datafusion/physical-plan/src/aggregates/mod.rs

@@ -2032,7 +2037,7 @@ mod tests {
        spill: bool,
    ) -> Result<()> {
        let task_ctx = if spill {
-            new_spill_ctx(2, 3200)
+            new_spill_ctx(2, 4200)


Again, seems like we need to make something configurable here?

I might be misunderstanding, do you mean the change in 3a33755 🤔 ?

andygrove

LGTM but I would like to understand the aggregate spilling changes more.

Thanks @waynexia

alamb · 2024-06-06T20:33:11Z

I merged up from main and updated the lock file

alamb

Thanks @waynexia -- I agree with @andygrove 's concern about the changes to spilling -- I think we should address them before merging but otherwise this looks really nice to me

🙏

datafusion/optimizer/src/analyzer/type_coercion.rs

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

waynexia · 2024-06-07T13:18:56Z

Thanks for reviewing!

abhiaagarwal · 2024-06-09T22:48:11Z

@waynexia I was inspecting the dependency tree for delta-rs and it seems the dependency didn't get bumped for object-store in proto-common fyi, so 0.9.1 gets pulled.

object_store v0.9.1
└── datafusion-proto-common v39.0.0 (https://github.com/apache/datafusion?tag=39.0.0-rc1#6a4a280e)
    └── datafusion-proto v39.0.0 (https://github.com/apache/datafusion?tag=39.0.0-rc1#6a4a280e)
        └── deltalake-core v0.18.0 (/Users/abhiagarwal/Developer/delta-rs/crates/core)
            ├── deltalake v0.18.0 (/Users/abhiagarwal/Developer/delta-rs/crates/deltalake)
            │   └── deltalake-python v0.18.0 (/Users/abhiagarwal/Developer/delta-rs/python)
            ├── deltalake-aws v0.1.2 (/Users/abhiagarwal/Developer/delta-rs/crates/aws)
            │   └── deltalake v0.18.0 (/Users/abhiagarwal/Developer/delta-rs/crates/deltalake) (*)
            ├── deltalake-azure v0.1.2 (/Users/abhiagarwal/Developer/delta-rs/crates/azure)
            │   └── deltalake v0.18.0 (/Users/abhiagarwal/Developer/delta-rs/crates/deltalake) (*)
            ├── deltalake-catalog-glue v0.1.0 (/Users/abhiagarwal/Developer/delta-rs/crates/catalog-glue)
            │   └── deltalake v0.18.0 (/Users/abhiagarwal/Developer/delta-rs/crates/deltalake) (*)
            ├── deltalake-gcp v0.2.1 (/Users/abhiagarwal/Developer/delta-rs/crates/gcp)
            │   └── deltalake v0.18.0 (/Users/abhiagarwal/Developer/delta-rs/crates/deltalake) (*)
            └── deltalake-mount v0.1.0 (/Users/abhiagarwal/Developer/delta-rs/crates/mount)
                └── deltalake-python v0.18.0 (/Users/abhiagarwal/Developer/delta-rs/python)

waynexia · 2024-06-10T00:50:56Z

Sorry for causing this! Opened #10848 to fix it.

abhiaagarwal · 2024-06-10T03:39:51Z

All good, just wanted to bring it to your attention before 39 formally drops :)

alamb · 2024-06-10T15:33:32Z

All good, just wanted to bring it to your attention before 39 formally drops :)

FYI @andygrove -- maybe this means we should make another RC for 39.0.0 🤔

We could also potentially make a 39.1.0 release too with this dependency upgrade fix #10848

…pache#10765) * fix compile on default feature config Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * fix test of common, functions, optimizer and physical-expr Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * fix other tests Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * fix one last test Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * fix clippy warnings Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * fix datafusion-cli Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * switch to git deps Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * regen proto file Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * fix pyo3 feature Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * fix slt Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * fix symmetric hash join cases Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * update integration result Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * fix up spill test Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * shift to the released packages Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * Update cargo.lock * Update datafusion/optimizer/src/analyzer/type_coercion.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * update document Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * move memory limit to parameter pos Signed-off-by: Ruihang Xia <waynestxia@gmail.com> --------- Signed-off-by: Ruihang Xia <waynestxia@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

fix compile on default feature config

2dac471

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions substrait labels Jun 2, 2024

This was referenced Jun 2, 2024

feat: add substrait support for Interval types and literals #10646

Merged

Release DataFusion 39.0.0 #10517

Closed

fix test of common, functions, optimizer and physical-expr

43acb9b

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

github-actions bot added optimizer Optimizer rules core Core DataFusion crate labels Jun 3, 2024

waynexia added 6 commits June 3, 2024 20:22

fix other tests

292a311

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

fix one last test

e90bd75

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

fix clippy warnings

3988449

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

fix datafusion-cli

5923709

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

switch to git deps

fe806e4

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

regen proto file

6fb71d7

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

tustvold mentioned this pull request Jun 3, 2024

Prepare arrow 52.0.0 apache/arrow-rs#5836

Merged

andygrove mentioned this pull request Jun 3, 2024

Switch back to released version of DataFusion and arrow-rs after Arrow Java 16 is released apache/datafusion-comet#248

Closed

waynexia added 2 commits June 4, 2024 19:59

fix pyo3 feature

eb65e18

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

fix slt

d13a9fe

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jun 4, 2024

waynexia added 3 commits June 5, 2024 11:54

fix symmetric hash join cases

fc9a86b

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

update integration result

96aff58

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

fix up spill test

2bf7813

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

github-actions bot added the sql SQL Planner label Jun 5, 2024

waynexia marked this pull request as ready for review June 5, 2024 09:30

shift to the released packages

16971c2

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

andygrove reviewed Jun 6, 2024

View reviewed changes

datafusion/common/src/scalar/mod.rs Outdated Show resolved Hide resolved

andygrove reviewed Jun 6, 2024

View reviewed changes

andygrove approved these changes Jun 6, 2024

View reviewed changes

alamb added 2 commits June 6, 2024 16:32

Merge remote-tracking branch 'apache/main' into prepare-arrow-update

6846513

Update cargo.lock

2645bee

alamb mentioned this pull request Jun 6, 2024

Update object_store requirement from 0.9.1 to 0.10.1 #10694

Closed

alamb approved these changes Jun 6, 2024

View reviewed changes

datafusion/optimizer/src/analyzer/type_coercion.rs Outdated Show resolved Hide resolved

waynexia and others added 3 commits June 7, 2024 09:46

Update datafusion/optimizer/src/analyzer/type_coercion.rs

f41263c

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

update document

34e8923

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

move memory limit to parameter pos

3a33755

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

andygrove merged commit cb9068c into apache:main Jun 7, 2024
25 checks passed

waynexia deleted the prepare-arrow-update branch June 7, 2024 13:18

waynexia mentioned this pull request Jun 10, 2024

MINOR: use workspace deps in proto-common (upgrade object store dependency) #10848

Merged

Omega359 mentioned this pull request Jun 11, 2024

Upgrade object_store to 0.10.1 #10673

Closed

This was referenced Jun 11, 2024

DataFusion weekly project plan (Andrew Lamb) - June 10, 2024 #10869

Closed

DataFusion weekly project plan (Andrew Lamb) - June 17, 2024 #10955

Closed

alamb mentioned this pull request Jun 26, 2024

Prepare object_store 0.10.0 #10116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build(deps): update Arrow/Parquet to `52.0`, object-store to `0.10` #10765

build(deps): update Arrow/Parquet to `52.0`, object-store to `0.10` #10765

waynexia commented Jun 2, 2024

waynexia commented Jun 5, 2024

alamb commented Jun 5, 2024

andygrove commented Jun 6, 2024

andygrove Jun 6, 2024

andygrove Jun 6, 2024

waynexia Jun 7, 2024

andygrove Jun 7, 2024

andygrove Jun 6, 2024

waynexia Jun 7, 2024

andygrove left a comment

alamb commented Jun 6, 2024

alamb left a comment

waynexia commented Jun 7, 2024

abhiaagarwal commented Jun 9, 2024

waynexia commented Jun 10, 2024

abhiaagarwal commented Jun 10, 2024

alamb commented Jun 10, 2024

	let expected = if spill {
	vec![
	"+---+---------------+-------------+",
	"\| a \| AVG(b)[count] \| AVG(b)[sum] \|",
	"+---+---------------+-------------+",
	"\| 2 \| 1 \| 1.0 \|",
	"\| 2 \| 1 \| 1.0 \|",
	"\| 3 \| 1 \| 2.0 \|",
	"\| 3 \| 2 \| 5.0 \|",
	"\| 4 \| 3 \| 11.0 \|",
	"+---+---------------+-------------+",
	]
	} else {

	pub struct GroupValuesPrimitive<T: ArrowPrimitiveType> {
	/// The data type of the output array
	data_type: DataType,
	/// Stores the group index based on the hash of its value
	///
	/// We don't store the hashes as hashing fixed width primitives
	/// is fast enough for this not to benefit performance
	map: RawTable<usize>,
	/// The group index of the null value if any
	null_group: Option<usize>,
	/// The values for each group index
	values: Vec<T::Native>,
	/// The random state used to generate hashes
	random_state: RandomState,
	}

build(deps): update Arrow/Parquet to 52.0, object-store to 0.10 #10765

build(deps): update Arrow/Parquet to 52.0, object-store to 0.10 #10765

Conversation

waynexia commented Jun 2, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

waynexia commented Jun 5, 2024

alamb commented Jun 5, 2024

andygrove commented Jun 6, 2024

andygrove Jun 6, 2024

Choose a reason for hiding this comment

andygrove Jun 6, 2024

Choose a reason for hiding this comment

waynexia Jun 7, 2024

Choose a reason for hiding this comment

andygrove Jun 7, 2024

Choose a reason for hiding this comment

andygrove Jun 6, 2024

Choose a reason for hiding this comment

waynexia Jun 7, 2024

Choose a reason for hiding this comment

andygrove left a comment

Choose a reason for hiding this comment

alamb commented Jun 6, 2024

alamb left a comment

Choose a reason for hiding this comment

waynexia commented Jun 7, 2024

abhiaagarwal commented Jun 9, 2024

waynexia commented Jun 10, 2024

abhiaagarwal commented Jun 10, 2024

alamb commented Jun 10, 2024

build(deps): update Arrow/Parquet to `52.0`, object-store to `0.10` #10765

build(deps): update Arrow/Parquet to `52.0`, object-store to `0.10` #10765